Skip to content

feat!: Refactor client constructor to use options pattern#4201

Open
stevehipwell wants to merge 6 commits into
google:masterfrom
stevehipwell:client-options-pattern
Open

feat!: Refactor client constructor to use options pattern#4201
stevehipwell wants to merge 6 commits into
google:masterfrom
stevehipwell:client-options-pattern

Conversation

@stevehipwell
Copy link
Copy Markdown
Contributor

@stevehipwell stevehipwell commented May 8, 2026

This PR refactors the client to be immutable and to use the with options pattern for construction. This is a significant change but it should reduce the overall complexity and support better UX around new capabilities and backwards compatibility (e.g. github.WithAPIVersion("2026-03-10") or github.WithGHESVersion("3.15")).

  • Client is immutable
    • NewClient is the only way to create a new Client
    • After a Client is created if you need to create a new one you can use Clone
    • You no longer need to create multiple clients and their associated child structs to chain helpers
  • Client configuration is explicit
    • Options are handled in a specific order to limit the potential for misconfiguration
    • Passing invalid values into NewClient causes an early error instead of waiting for a failure or silently doing nothing
  • Fixed implementation bugs
    • clientMu was unnecessary
    • Proxy from env pattern lost default transport configuration
  • Improved test coverage?

Closes #3870
Closes #3915

@stevehipwell
feat!: refactor client constructor to use options pattern
7e6779b 
Signed-off-by: Steve Hipwell <steve.hipwell@gmail.com>
@gmlewis gmlewis changed the title feat!: refactor client constructor to use options pattern feat!: Refactor client constructor to use options pattern May 8, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented May 8, 2026 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.77%. Comparing base (6c58592) to head (de722e1).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #4201      +/-   ##
==========================================
+ Coverage   93.71%   97.77%   +4.05%     
==========================================
  Files         209      189      -20     
  Lines       19772    19035     -737     
==========================================
+ Hits        18529    18611      +82     
+ Misses       1046      228     -818     
+ Partials      197      196       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

gmlewis
gmlewis requested changes May 8, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @stevehipwell.

Although I like the overall pattern, I have a few concerns with this PR.
First, the various fields within Client were originally exported individually, on a case-by-case basis, as users had need to access them. Suddenly unexporting all fields may wreak havoc.

Second, the clientMu was indeed needed to solve a race condition found when copying clients that were currently in-flight, if I'm remembering correctly.

Can we keep the nice builder pattern you've created here without unexporting all the fields and without removing the mutex that is protecting the client?

Comment thread example/basicauth/main.go Outdated
Comment thread example/contents/main.go Outdated
Comment thread example/ratelimit/main.go Outdated
Comment thread example/tokenauth/main.go Outdated
Comment thread example/uploadreleaseassetfromrelease/main.go Outdated
Comment thread test/fields/fields.go Outdated
Comment thread test/fields/fields.go Outdated
Comment thread test/integration/github_test.go Outdated
Comment thread test/integration/github_test.go Outdated
Comment thread tools/metadata/main.go
@stevehipwell
Copy link
Copy Markdown
Contributor Author

stevehipwell commented May 8, 2026

@gmlewis if you look at the code you'll see that clientMu is doing nothing and doesn't even wrap the relevant concerns in the copy function. It used to technically serve a purpose when some of the functions mutated the client, but that no longer happens, although as I said above it wasn't "correct" even for that. I also double checked this with 2 different agents, which both agreed with my summary above.

Unexporting the values and putting them behind the options pattern to make the client immutable is core I the whole pattern. Could you provide some examples where these need changing and Clone is the wrong choice? For example I added the disable rate limit option to the client, it's only exported as there wasn't an existing pattern so that was the simplest approach. I'm happy to add getters for the remaining unexpired fields, but I wasn't sure they were actually needed.

@gmlewis
Copy link
Copy Markdown
Collaborator

gmlewis commented May 9, 2026

@gmlewis if you look at the code you'll see that clientMu is doing nothing and doesn't even wrap the relevant concerns in the copy function. It used to technically serve a purpose when some of the functions mutated the client, but that no longer happens, although as I said above it wasn't "correct" even for that. I also double checked this with 2 different agents, which both agreed with my summary above.

Unexporting the values and putting them behind the options pattern to make the client immutable is core I the whole pattern. Could you provide some examples where these need changing and Clone is the wrong choice? For example I added the disable rate limit option to the client, it's only exported as there wasn't an existing pattern so that was the simplest approach. I'm happy to add getters for the remaining unexpired fields, but I wasn't sure they were actually needed.

OK, I see. That sounds good to me.

If you wouldn't mind please cleaning up all the non-idiomatic failures in the examples where I have marked them, that would be greatly appreciated, then we should be ready for a second LGTM+Approval.

@gmlewis
Copy link
Copy Markdown
Collaborator

gmlewis commented May 9, 2026

If you could also please investigate the 8 missed lines in code coverage in github/github.go in this PR, that would also be great. Thanks!

@gmlewis gmlewis added NeedsReview PR is awaiting a review before merging. Breaking API Change PR will require a bump to the major version num in next release. Look here to see the change(s). labels May 9, 2026
alexandear
alexandear reviewed May 11, 2026
View reviewed changes
Comment thread README.md
Comment thread README.md
Comment thread README.md
Comment thread github/github.go Outdated
Comment thread github/github.go Outdated
Comment thread github/github_test.go
Comment thread github/github_test.go Outdated
Comment thread github/github_test.go Outdated
Comment thread github/github_test.go Outdated
alexandear
alexandear reviewed May 11, 2026
View reviewed changes
Comment thread github/github_test.go Outdated
@stevehipwell
Copy link
Copy Markdown
Contributor Author

stevehipwell commented May 11, 2026

@gmlewis I've addressed you review comments and should have fixed the test coverage. I've also added exclusions for the examples and other code that shouldn't be be part of the coverage report which should make it easier to see what code should have been tested but hasn't been.

Not-Dhananjay-Mishra
Not-Dhananjay-Mishra reviewed May 11, 2026
View reviewed changes
Comment thread github/examples_test.go Outdated
@stevehipwell
Copy link
Copy Markdown
Contributor Author

stevehipwell commented May 12, 2026

@alexandear I'm still waiting on a reply on a couple of threads you opened?

@stevehipwell
Copy link
Copy Markdown
Contributor Author

stevehipwell commented May 12, 2026

@gmlewis all review comments should now have been addressed.

@gmlewis
Copy link
Copy Markdown
Collaborator

gmlewis commented May 12, 2026

@gmlewis all review comments should now have been addressed.

Thanks, @stevehipwell.
It looks like conflicts must be resolved before we can move forward.

@gmlewis gmlewis removed the NeedsReview PR is awaiting a review before merging. label May 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gmlewis gmlewis gmlewis requested changes

+2 more reviewers

@alexandear alexandear alexandear approved these changes

@Not-Dhananjay-Mishra Not-Dhananjay-Mishra Not-Dhananjay-Mishra left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Breaking API Change PR will require a bump to the major version num in next release. Look here to see the change(s).

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Question on DownloadReleaseAsset redirect logic Refactor WithEnterpriseURLs helper

4 participants

@stevehipwell @gmlewis @alexandear @Not-Dhananjay-Mishra